-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[New] add jsx-no-leaked-render
#3203
[New] add jsx-no-leaked-render
#3203
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3203 +/- ##
=======================================
Coverage 97.67% 97.68%
=======================================
Files 121 122 +1
Lines 8575 8627 +52
Branches 3112 3124 +12
=======================================
+ Hits 8376 8427 +51
- Misses 199 200 +1
Continue to review full report at Codecov.
|
I'm not sure how to fix the 2 remaining checks failing. |
@yannickcr pinging you for getting some review. |
@Belco90 there's no need for pings; if you haven't gotten a review yet, it's because nobody's had time, not because the notifications we already get from the PR itself were missed. |
@ljharb my bad, sorry for any inconvenience caused. |
This may also solve some/most of #2073. |
7d9713e
to
3bbdea6
Compare
3bbdea6
to
04472dd
Compare
02efc2d
to
6c6a49a
Compare
Everything finally sorted out! |
Codecov Report
@@ Coverage Diff @@
## master #3203 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 122 123 +1
Lines 8667 8719 +52
Branches 3148 3160 +12
=======================================
+ Hits 8468 8519 +51
- Misses 199 200 +1
Continue to review full report at Codecov.
|
I've renamed the rule to
|
ad3589f
to
95883de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rebased this; it's almost there
I'll fix the failing tests and apply the suggested changes later. Thanks! |
95883de
to
6b7b191
Compare
Not sure why the "Automatic Rebase" check keeps failing |
because instead of deleting your local branch and checking out my rebase, you merged your original local branch into my rebased one, and that means there's merge conflicts when trying to rebase it. Don't worry about that tho, I can handle it when it's time to land this. |
e565c15
to
ef733fd
Compare
jsx-no-leaked-render
@Belco90 @ljharb is this rule description still valid for React Native?
I can't get an empty string to crash React Native without a wrapping I would do the PR to update the docs if this is no longer a thing... |
That would be a major DX change for React Native, so I'd want to see some kind of announcement that it was intentional by the RN team. |
I also was surprised - but couldn't find anything official after a bit of digging. Asking some maintainers here: https://twitter.com/karlhorky/status/1576987525914464256 |
Ok, so I didn't really get a clear answer from any maintainers there, but I just started a new React Native app just now ( Whereas a string with content in it does: Repo: https://github.com/karlhorky/react-native-empty-string-outside-text-component @ljharb enough proof? |
Asked some more maintainers about whether this is new behavior: https://twitter.com/karlhorky/status/1577257935796740096 |
In which RN version did this change? To be clear, I’m not looking for proof it changed, I’m looking for proof it was intentional :-) |
Good point about it being intentional, @rickhanlonii is going to look into it: https://twitter.com/rickhanlonii/status/1577291418485362688 |
Since I haven't received any feedback yet, I opened an issue on the React Native GitHub repo: facebook/react-native#35002 |
Looks like indeed an intentional change - actually in React itself (in version 18): facebook/react-native#35002 (comment) |
@karlhorky sounds like we need a PR to update the docs, as well as the rule when react is >= 18 |
Ok, opened #3479 for the rule change |
New rule
jsx-no-leaked-zero
to prevent potential 0 leaked to the DOM from numeric inline conditions. It includes 2 autofix strategies.